feat(baggage): Added double baggage prevention logic#19597
feat(baggage): Added double baggage prevention logic#19597harshit078 wants to merge 15 commits intogetsentry:developfrom
Conversation
|
Hey @harshit078 thanks for taking over the issue and opening the PR! I see there are some tests failing regarding the There's one test named "overwrites existing Sentry entries with new ones", so I'm wondering why we'd end up with duplicated entries. This might be related to #17355 which got merged yesterday. |
|
Hey @Lms24 , yes I am resolving the current failing tests and pushing a fix for it. I'll mark the PR as ready to review as soon as all tests pass |
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛Deps
Other
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| const baggageString = Array.isArray(baggageHeader) ? baggageHeader.join(',') : baggageHeader; | ||
|
|
||
| return baggageString.split(',').some(entry => entry.trim().startsWith(SENTRY_BAGGAGE_KEY_PREFIX)); | ||
| } |
There was a problem hiding this comment.
New hasSentryBaggageValues function is unused in production
Medium Severity
The newly exported hasSentryBaggageValues function is never imported or called in any production code. Only mergeBaggageHeaders is imported from this module (by outgoingFetchRequest.ts and outgoingHttpRequest.ts). Given the PR's goal of "double baggage prevention," this function appears intended to guard against adding duplicate sentry baggage entries in the outgoing request handlers — similar to how baggageHeaderHasSentryBaggageValues is used in packages/core/src/fetch.ts — but the wiring was never added. This likely means the feature is incomplete.
| expect(results.test4.hasCustomBaggage).toBe(true); | ||
| expect(results.test4.sentryTrace).toBeDefined(); | ||
| expect(results.test4.baggage).toContain('custom-key=value'); | ||
| expect(results.test4.sentryBaggageCount).toBeGreaterThan(0); |
There was a problem hiding this comment.
Integration test doesn't verify baggage deduplication was prevented
Medium Severity
The integration test for "double baggage prevention" uses toBeGreaterThan(0) for sentryBaggageCount, which would pass even if sentry baggage entries were duplicated 10x. The test expects duplicate entries NOT to exist but never actually asserts this. For test1/test3/test4 (the manual + auto-instrumentation scenarios), the sentryBaggageCount needs to be compared against the expected unique count, or the test2 baseline count, to actually verify deduplication. As written, these assertions don't test the newly added behavior.
Triggered by project rule: PR Review Guidelines for Cursor Bot
2 participants
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Closes #19158